Repository navigation
Fix confirmed findings from the #389 adversarial review - #414
Merged
Merged
Conversation
The multi-agent review of PR #389 (incident forensics hardening) confirmed 15 findings, 6 with real behavioral impact. All are addressed here, plus the load-bearing comment rot it flagged. feedDebugLog.ts (5 behavioral fixes): * Pre-existing over-cap files never got a tombstone. Priming tombstoneWritten from `startingBytes >= cap` routed the first post-upgrade batch into the already-capped short-circuit, which never writes a marker — so the 60-300 MiB pre-cap-era files that MOTIVATED the cap (#388) had appends dropped silently. Now primes false; the first batch flows through the cap branch, writes the tombstone, then latches. * Tombstone write failure silently latched as "capped". The bare catch around the tombstone append still set tombstoneWritten=true, so an ENOSPC (the exact pressure the cap exists for) produced a file with no marker that the code believed was cleanly marked. Now latches only on success; on failure the next batch retries the tombstone (one small append attempt per batch). * stat() failures failed OPEN at 0 bytes. Any non-ENOENT error (EACCES, EIO, EMFILE — fd exhaustion is likeliest under exactly the load the cap targets) primed the counter at 0 and let an already-300 MiB file grow by another full cap. loadInitialFileBytes now returns null for unknown-size; the caller fails CLOSED — unprimes and returns without advancing the cursor so the renderer resends and the next batch re-stats. * forgetFeedDebugSession racing the stat() await leaked map entries. get → await → set meant a forget during the await deleted a not-yet-existing key, then the continuation inserted state for a dead session — permanent leak since session UUIDs never recur. The placeholder now lands in the map BEFORE the await; an identity re-check after it detects the forget and bails. * On-disk droppedEntriesSoFar froze at the first batch's count. Capped sessions kept flooding for hours while the tombstone reported the first few hundred drops. A refreshed tombstone row is now appended each time total drops DOUBLE since the last row — log-bounded (~20 rows for a million drops) and the last row is always within 2x of the truth. * Batch drops were all-or-nothing. A 40 MiB batch whose LAST bytes crossed the cap discarded all 40 MiB even though most entries fit. Entries are now serialized per-line and the cap cuts WITHIN the batch: the admitted prefix is written in the same append as the tombstone (JSONL prefixes are always valid; no torn interleaving). previousRunClassifier.ts: * main_oom_suspected dropped minidumpPath — a regression. V8 fatal aborts raise SIGABRT, so Crashpad writes a minidump for the same crash the diagnostic report describes. The new OOM branch returned before the minidump lookup, so evidence lost `native`/`minidumpPath` for exactly the crash class the branch was added to improve, and downstream symbolication read undefined. The OOM branch now runs findRecentMinidump too. * Fixed both "trigger is a top-level string" comments: trigger lives under `header` in Node diagnostic reports. The regex works because it is a flat text scan; the comments now say so explicitly and warn against "simplifying" to JSON.parse(prefix).trigger, which returns undefined and would silently kill every OOM classification. heapWatchdog.ts: * The threshold comment claimed the ~1.2 GB abort class "was already covered by the ratio" — algebraically false on the 2026-05-11 host (ratio arm resolves to ~3.0 GiB; both old and new trips sit above 1.2 GB). Replaced with an honest coverage note: low-peak aborts are attributed post-hoc by reportOnFatalError + the prior-run classifier; polling only buys a pre-crash snapshot for climbs crossing ~1.5 GiB, and why the trip is not lowered further. index.ts: * setDebugRetentionJournal(appRunJournal) moved BEFORE appRunJournal.start(). start() fires the run's first prune (incident-run-start), and with the sink wired after the await the first prune's journal entry was race-free only because the prune's statfs() I/O happened to lose to the synchronous continuation — any future await inserted between them would silently drop the breadcrumb. Wiring first makes the ordering structural. The old comment (which misdescribed 'startup' as the first prune) is gone. Verification: applied to a clean main checkout — `tsc -b tsconfig.node.json` exit 0, `tsc --noEmit -p tsconfig.web.json` 0 errors. No new tests per repo convention. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The adversarial review's PLAUSIBLE verdicts were initially set aside; auditing all 16 against the confirmed-tier fixes found four real gaps that cost a few lines each: * AppRunJournal.start(): set process.report.directory BEFORE reportOnFatalError. In the old order, a throw from the directory setter left the flag armed with Node's default directory (CWD), so a real OOM wrote its report where the prior-run classifier never looks — silently reverting to force_quit_or_power_loss. Directory- first means a throw leaves the flag off entirely; the minidump path still attributes the crash. * AppRunJournal.stop(): latch started=false. record() kept accepting events after stop(), and the debug-retention prune chain holds this journal as its sink for the process lifetime — a late .then in the will-quit window pushed events into `pending` with every drain timer already cleared. Dead weight, never written. * previousRunClassifier: match Node's real report name shape (report.<date>.<time>.<pid>.<tid>.<seq>.json) instead of loose report.*.json. A future artifact named e.g. report.summary.json sorted lexically last, won the newest-pick, had no trigger — and shadowed the genuine fatal-error report sitting next to it. * previousRunClassifier: slice the prefix buffer to readSync's actual return value instead of stringifying the whole zero-filled allocation on a short read. Remaining PLAUSIBLE findings intentionally NOT acted on: the regex- anchoring concerns (guarded by JSON escaping + header-first ordering in every known Node version; now documented at the regex), the missing prune call in the already-capped branch (no new bytes → no new information, and the 5-minute cooldown makes it moot), and the trigger-unreadable fallthrough (nodeReportPath still lands in evidence, so triage sees the report even when classification stays conservative). Verification: full branch diff applied to a clean main checkout — tsc -b node exit 0, tsc web 0 errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #389 (incident forensics hardening). The multi-agent adversarial review of that PR confirmed 15 findings; this fixes all 6 with behavioral impact plus the load-bearing comment rot. One PR, per "no micro-PRs" convention.
Behavioral fixes
src/main/storage/feedDebugLog.ts— 5 fixes to the per-file capPre-existing over-cap files never got a tombstone. Priming
tombstoneWrittenfromstartingBytes >= caprouted the first post-upgrade batch into the already-capped short-circuit, which never writes a marker — so the 60–300 MiB pre-cap-era files that motivated the cap (Incident forensics hardening: catch fast heap bursts, cap feed-debug, journal retention, detect prior-run OOMs #388) had their appends dropped with zero on-disk trace. Now primesfalse; the first batch flows through the cap branch, writes the tombstone, then latches.Tombstone write failure silently latched as "capped". The bare
catch {}still settombstoneWritten = true, so an ENOSPC — disk-full being the exact pressure the cap exists for — produced a file with no marker that the code believed was cleanly marked. Now latches only on success; on failure the next batch retries (one small append attempt per batch).stat()failures failed OPEN at 0 bytes. Any non-ENOENT error (EACCES/EIO/EMFILE — fd exhaustion is likeliest under exactly the load the cap targets) primed the counter at 0 and let an already-300 MiB file grow by another full cap's worth.loadInitialFileBytesnow returnsnullfor unknown-size; the caller fails CLOSED — unprimes and returns without advancing the cursor, so the renderer resends and the next batch re-stats.forgetFeedDebugSessionracing thestat()await leaked map entries. get → await → set meant a forget firing during the await deleted a not-yet-existing key (no-op), then the continuation inserted cap state for a dead session — a permanent leak since session UUIDs never recur. The placeholder now lands in the map BEFORE the await, and an identity re-check after it detects the forget and bails.On-disk
droppedEntriesSoFarfroze at the first batch's count while capped sessions kept flooding for hours. A refreshed tombstone row is now appended each time the total doubles since the last row — log-bounded (~20 rows for a million drops), and the last row is always within 2× of the truth.Batch drops were all-or-nothing. A 40 MiB batch whose last bytes crossed the cap discarded all 40 MiB even though most entries fit. Entries are now serialized per-line and the cap cuts within the batch; the admitted prefix is written in the same
writeFileappend as the tombstone (JSONL prefixes are always valid, so no torn interleaving is possible).src/main/incident/previousRunClassifier.ts— minidump regressionmain_oom_suspecteddroppedminidumpPathfrom evidence. V8 fatal aborts raise SIGABRT, so Crashpad almost always writes a minidump for the same crash the diagnostic report describes. The new OOM branch returned before the minidump lookup ran — losingnative/minidumpPathfor exactly the crash class the branch was added to improve (pre-#389 code surfaced both). The OOM branch now runsfindRecentMinidumptoo.Comment/structure fixes
previousRunClassifier.ts— both "trigger is a top-level string" comments were factually wrong (triggerlives underheader). Comments now describe the real shape and explicitly warn against "simplifying" the flat regex scan toJSON.parse(prefix).trigger, which returnsundefinedand would silently kill every OOM classification.heapWatchdog.ts— the claim that the ~1.2 GB abort class "was already covered by the ratio" was algebraically false (ratio arm resolves to ~3.0 GiB on the 2026-05-11 host; both old and new trips sit above 1.2 GB). Replaced with an honest coverage note: low-peak aborts are attributed post-hoc viareportOnFatalError+ the classifier; polling only buys a pre-crash snapshot for climbs crossing ~1.5 GiB.index.ts—setDebugRetentionJournal(appRunJournal)moved BEFOREappRunJournal.start().start()fires the run's first prune (incident-run-start), and with the sink wired after theawaitthe first prune's journal entry was race-free only by accident (the prune'sstatfs()I/O losing to the synchronous continuation). Any futureawaitinserted between them would have silently dropped the breadcrumb. Wiring first makes the ordering structural instead of incidental; the old comment (which also misidentified'startup'as the first prune) is gone.Verification
Applied to a clean
maincheckout:tsc -b tsconfig.node.jsonexit 0,tsc --noEmit -p tsconfig.web.json0 errors. No new tests per repo convention.🤖 Generated with Claude Code